-
Notifications
You must be signed in to change notification settings - Fork 525
TerminalShell (Linux): better support for Ghostty #1803
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
getenv("GHOSTTY_RESOURCES_DIR") != NULL || | ||
getenv("GHOSTTY_SHELL_INTEGRATION_NO_SUDO") != NULL || | ||
getenv("GHOSTTY_BIN_DIR") != NULL | ||
) term = "Ghostty"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ghostty reports $TERM_PROGRAM
, so additional env-var checks are not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Removed.
da1096c
to
535c877
Compare
d6fd359
to
16373f4
Compare
Rebased. Commit message has been updated. |
Hello. Please reply my comments first. |
|
@CarterLi So, where is your comment? Could you paste a screenshot like what I did? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review these comments
if(version->length > 0) return true; | ||
} | ||
|
||
if(!getExeVersionRaw(exe, version)) return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually dont think this is needed. As $TERMINAL_PROGRAM_VERSION exists.
We rely on it for some other terminals too, such as Wezterm, iTerm and warp.
Note we always add another detection method after ffBinaryExtractStrings usage, as ffBinaryExtractStrings relies on the internal implementation of terminals which is not very reliable. While $TERMINAL_PROGRAM_VERSION is considerred a public API; if the variable is not there it is a terminal bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (env) | ||
{ | ||
ffStrbufAppendS(version, env); | ||
// 1.1.3-arch1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI fedora version also includes .fc42
. Lets keep as it was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The kernel version contains archlinux specific suffix -arch1-1
. I don't think it's problematic. Do you?
Can you see it now? |
Thanks, I can see it now. Maybe it's Github's bug that it didn't display to me. |
The version suffix doesn't indicate a change in the program code, sometimes the suffix changes simply by packager recompiling it. Many other programs don't have the suffix in their versions output, such as bash (usually this is a side effect of the formatting), so I'm considering whether or not to remove ghostty's version suffix just the same. But since Fedora also has a version suffix, I think it's okay to keep it. I'll close this PR. Thanks for your review. |
Changes:
Ghostty does not support Windows (still WIP at an early stage), so only *nix parts modified.